Skip to content

[FIX] Fix AbstractSortTableModel.mapFromSourceRows for empty list or array#2730

Merged
kernc merged 1 commit intobiolab:masterfrom
janezd:fix-mapFromSourceRows
Nov 3, 2017
Merged

[FIX] Fix AbstractSortTableModel.mapFromSourceRows for empty list or array#2730
kernc merged 1 commit intobiolab:masterfrom
janezd:fix-mapFromSourceRows

Conversation

@janezd
Copy link
Copy Markdown
Contributor

@janezd janezd commented Nov 2, 2017

Issue

Fixes #2728.

Description of changes

Test whether a list is empty before using it for indexing an array.

Includes
  • Code changes
  • Tests

@janezd janezd force-pushed the fix-mapFromSourceRows branch from 1cbeae3 to 4ccaaec Compare November 2, 2017 10:22
# self.__sortInd[rows] fails if `rows` is an empty list or array
if self.__sortInd is not None \
and (isinstance(rows, (int, type(Ellipsis)))
or len(rows)):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kernc, what do you prefer:

  • (isinstance(rows, (int, type(Ellipsis))) or len(rows)),
  • (not isinstance(rows, (list, np.ndarray)) or len(rows)), or
  • (not isinstance(rows, collections.Sized) or len(rows))?

If the method is called with the argument types specified in the docstring, all are equivalent. Also, in cases where they are not equivalent (e.g. if rows is something stupid), the next line will fail anyway.

Copy link
Copy Markdown
Contributor

@kernc kernc Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already see:

>>> arr = np.ones(5)
>>> empty_list = []
>>> arr[empty_list]
array([], dtype=float64)

I.e. empty input, empty output.

You don't?

Copy link
Copy Markdown
Contributor

@kernc kernc Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer:

if self.__sortInd is not None:
    if rows is not Ellipsis:
        rows = np.asarray(rows, dtype=int)  # dangerous casting

    new_rows = self.__sortInd[rows]
    ...

But this also relies on indexing (at least) with an empty array working.

I am not aware of this ever not being the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer your first option. Note to also fix .mapFromSourceRows() below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dtype must not be float; the caller must ensure that. This is similar to __getitem__ which expects ints, and doesn't cast float to int. I made this clear in the docstring.

I fixed mapFromSourceRows().

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 2, 2017

Codecov Report

Merging #2730 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2730      +/-   ##
==========================================
+ Coverage   76.04%   76.05%   +<.01%     
==========================================
  Files         338      338              
  Lines       59643    59654      +11     
==========================================
+ Hits        45358    45370      +12     
+ Misses      14285    14284       -1

@kernc
Copy link
Copy Markdown
Contributor

kernc commented Nov 2, 2017

I think #2733 is a more correct fix.

@janezd janezd force-pushed the fix-mapFromSourceRows branch 4 times, most recently from 911f556 to 0f53884 Compare November 2, 2017 20:54
@janezd
Copy link
Copy Markdown
Contributor Author

janezd commented Nov 2, 2017

If I understood your comments (and their removals) correctly, this is complete now. I also improved the tests for the two mappings, so they now actually test something. :)

@kernc kernc force-pushed the fix-mapFromSourceRows branch from 0f53884 to 54c5e9f Compare November 3, 2017 01:20
@kernc
Copy link
Copy Markdown
Contributor

kernc commented Nov 3, 2017

Yes. Even so, I dared push some improvements while the build remains untamed.

@janezd janezd force-pushed the fix-mapFromSourceRows branch from 54c5e9f to d83130c Compare November 3, 2017 07:35
@janezd
Copy link
Copy Markdown
Contributor Author

janezd commented Nov 3, 2017

Feel free to dare. I force-pushed, hope it builds now. (Sorry for removing your name from the commit; it disappeared while resolving merge conflicts(?!).)

@janezd janezd force-pushed the fix-mapFromSourceRows branch 2 times, most recently from 6a25001 to 6d6ff33 Compare November 3, 2017 08:12
@janezd
Copy link
Copy Markdown
Contributor Author

janezd commented Nov 3, 2017

AppVeyor:

Ran 1895 tests in 146.438s
OK (skipped=122)
Command exited with code -1073741819

Anybody, help?

@kernc kernc force-pushed the fix-mapFromSourceRows branch from 6d6ff33 to fcb2da3 Compare November 3, 2017 11:38
@astaric
Copy link
Copy Markdown
Member

astaric commented Nov 3, 2017

This looks like a "segfault" to me, no idea why it happens though. It usually happens when an error occurs when destroying objects upon closing of the interpreter.

@kernc kernc force-pushed the fix-mapFromSourceRows branch from fcb2da3 to 5854208 Compare November 3, 2017 12:04
@kernc
Copy link
Copy Markdown
Contributor

kernc commented Nov 3, 2017

That's what one gets for squandering over sensible modifications.

@kernc kernc merged commit 727430c into biolab:master Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants